-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add missing methods in PyGraph and PyDiGraph stubs #967
Conversation
if TYPE_CHECKING: | ||
from .digraph import PyDiGraph | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyGraph and PyDiGraph have a "cyclic" dependency due to the to_directed
and to_undirected
method. This is a hack to make Python agree with the Rust code
Pull Request Test Coverage Report for Build 7513006606
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this LGTM, just inline comments about the mapping of Vec<usize>
to list[int]
as Vec<usize>
will take more than just a list.
Also is there a point where we can catch these missing functions in CI? I assume we're not yet because we're still marked as only partially typed. But can we enforce this for new methods on the graph classes while working on the rest of the functions?
rustworkx/digraph.pyi
Outdated
@@ -52,11 +57,20 @@ class PyDiGraph(Generic[S, T]): | |||
node_map_func: Callable[[S], int] | None = ..., | |||
edge_map_func: Callable[[T], int] | None = ..., | |||
) -> dict[int, int]: ... | |||
def contract_nodes( | |||
self, | |||
nodes: list[int], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically I think this is
nodes: list[int], | |
nodes: Sequence[int], |
although it also will except a numpy.ndarray
, which isn't a sequence, for all int dtypes I think.
rustworkx/digraph.pyi
Outdated
def copy(self) -> PyDiGraph[S, T]: ... | ||
def edge_index_map(self) -> EdgeIndexMap[T]: ... | ||
def edge_indices(self) -> EdgeIndices: ... | ||
def edge_list(self) -> EdgeList: ... | ||
def edges(self) -> list[T]: ... | ||
def edge_subgraph(self, edge_list: list[tuple[int, int]], /) -> PyDiGraph[S, T]: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above with contract_nodes
.
def edge_subgraph(self, edge_list: list[tuple[int, int]], /) -> PyDiGraph[S, T]: ... | |
def edge_subgraph(self, edge_list: Sequence[tuple[int, int]], /) -> PyDiGraph[S, T]: ... |
rustworkx/graph.pyi
Outdated
@@ -48,12 +55,20 @@ class PyGraph(Generic[S, T]): | |||
node_map_func: Callable[[S], int] | None = ..., | |||
edge_map_func: Callable[[T], int] | None = ..., | |||
) -> dict[int, int]: ... | |||
def contract_nodes( | |||
self, | |||
nodes: list[int], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as on digraph here.
We need to remove the Line 79 in 75dbca9
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the quick update.
Closes #1036
Related to #352
Adds methods that were added after #401 was merged. All of them were checked with
mypy.stubtest